-
Notifications
You must be signed in to change notification settings - Fork 22
add support for datetime database functions #37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
6cfb08e
to
28877d2
Compare
b784710
to
4d59f1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Left a couple questions of commentary.
@@ -63,7 +63,7 @@ jobs: | |||
- name: Start MongoDB | |||
uses: supercharge/[email protected] | |||
with: | |||
mongodb-version: 4.4 | |||
mongodb-version: 5.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reservation to running these tests using at least mongodb version 6.0? Or is the goal to test with our oldest supported version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4.4 was copied from the pymongo config. I bumped it here because $dateTrunc
is new in 5.0. For this CI, I'd suggest using the oldest version we decide to support. That choice doesn't necessarily have to follow MongoDB's supported versions, but thus far, I haven't encountered anything that makes supporting 5.0 more difficult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. My one hesitation is we may eventually run into a functionality only available in later versions. With that being said, we'll cross that bridge when we get there. I'll file an issue ticket to identify the min-supported version.
ExtractMinute.lookup_name: "minute", | ||
ExtractMonth.lookup_name: "month", | ||
ExtractSecond.lookup_name: "second", | ||
ExtractWeek.lookup_name: "isoWeek", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the switch toisoWeek
over week?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Django's ExtractWeek
is documented as returning 1-52 or 53, based on ISO-8601, i.e., Monday is the first of the week.
@@ -133,3 +134,43 @@ def _prep_lookup_value(self, value, field, field_kind, lookup): | |||
if field_kind == "DecimalField": | |||
value = self.adapt_decimalfield_value(value, field.max_digits, field.decimal_places) | |||
return value | |||
|
|||
"""Django uses these methods to generate SQL queries before it generates MQL queries.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we make these SQL queries just for logging/debugging purposes? Could we get away with not rendering them at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we cannot avoid it. It has to do with how this library hooks in to the query generation process. See also:
as_mql_agg() only needs to be defined if it differs from as_mql()
This feature flag has been added to mongodb-forks/django.
fixes #9